Encrypt GitHub access tokens #11585
Encrypt GitHub access tokens #11585
Conversation
Turbo87
added
C-enhancement ✨
|
Not something to block this PR, but we should think about what would be needed to support key rotation here in the future. Using a KMS would make this easier, but if we want to keep this all localized there are some options.
Decryption operations would perform the lookup via the |
|
Without doing a full review, my gut reaction is the same as @mdtro's — I'd prefer to use a KMS for rotation and logging purposes. Not going to call it a blocker just yet, but I do want think about this some more. @walterhpearce, you've been thinking about this kind of problem recently in the context of signing things. Any thoughts? |
|
I'm not opposed to using a KMS, but I have zero experience with it. You are both welcome to work on it and coordinate the deployment with the infra team though 😅 I'm a bit worried that we're letting "perfect" be the enemy of "good" here (or whatever the saying is). I would assume that we could still retrofit a KMS later even with the current design. We also have a few other keys that should maybe be moved into a KMS, so this seems like a bigger task that shouldn't necessarily prevent this quick win proposed here. Aside from that, the use of per-user GitHub access tokens might potentially turn out to not be viable for us anymore in the future. Once we support other identity providers then not every user might have a corresponding GitHub OAuth token anymore, in which case we might not even need to save them since we will need to use some other method anyway. In other words, I currently don't expect us to keep these around for the next ten years, so key rotation might not be quite as critical? |
|
so how do we continue here? should we ship this as an incremental improvement? or does one of you want to look into the KMS thing? |
|
|
We currently save the GitHub OAuth access tokens in plaintext in our database, which isn't a huge issue as long as nobody unauthorized gains access to our database. But from a security standpoint it's better to encrypt them so that an attacker would need both access to the database and to the encryption key.
This PR implements step 1 of the migration towards using encrypted access tokens. It adds a new column to the database, changes the code to start saving encrypted tokens to that column and adds an admin tool to backfill the column based on the current plaintext tokens.
This also adds a new required
GITHUB_TOKEN_ENCRYPTION_KEYenvironment variable without which the server and background worker won't start. The env var expects a 64 character hex string, which is then decoded to a 32 byte AES key. I've already generated corresponding values for our staging and production environments viaopenssl rand -hex 32.The encryption uses "AES-256-GCM" which from my understanding should be a reasonable algorithm for this situation and requires no extra dependencies since it was already used transitively.
For completeness: Phase 2 will turn the column non-nullable and will start using the encrypted tokens instead of the plaintext tokens. Phase 3 will then remove the plaintext token column. These phases should be deployed separately for obvious reasons 😅